Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bump vite to 5.4.14 #2312

Merged
merged 22 commits into from
Jan 30, 2025
Merged

Bump vite to 5.4.14 #2312

merged 22 commits into from
Jan 30, 2025

Conversation

r100-stack
Copy link
Member

@r100-stack r100-stack commented Oct 23, 2024 •
edited
Loading

Changes

After PR TODO from #2256 (#2256 (comment)).

Fixes GHSA-vg6x-rcgg-rjx6. Closed #2409 in favor of this PR.

Bumps vite from ~5.1 to ~5.4.14 (after deduping, only 5.4.14 is used atm).

After the bump, the styles.css was not getting generated in styles.js/dist. Looks like it is because of vitejs/vite/#16051 from 5.2.0-beta.0.

To fix it, I added the vite plugin suggested in vitejs/vite#16051 (comment). I had tried it when I had opened this PR too. However, it didn't work back then (which is why I had tried another fix which posed other problems; more info below). But since this vite plugin works now, I went ahead with this.

Old fix

To fix it, I renamed styles.module.css to styles.css. If there is a reason why the file has to be a module.css file, then we'd have to import and use the styles somewhere, or something along these lines.

One problem with using styles.css, however, is that the iui- prefix is not there in the generated styles.css file because the css module part of the vite config file is not applied.

Since I noticed multiple vite versions, especially some that were still in the affected range of the security advisory, I also deduped it to only use 5.4.14 (b50db2a). But this caused the css-workshop tests to all fail. To fix it, I changed the test script to replace astro preview with serve and added a wait-on step so that the tests start up only after serve has started up the server. (more info).

Testing

  • CI still passing
  • The build script is working in the itwinui-react workspace
  • The generated styles.css in packages/itwinui-react is unchanged.

Docs

No changeset as vite is used in dev workspaces or listed as devDependencies in the package workspaces (itwinui-react).

@r100-stack r100-stack self-assigned this Oct 23, 2024
@r100-stack r100-stack changed the title Bump vite to ~5.4.10 Bump vite to ~5.4.14 Jan 22, 2025
@r100-stack r100-stack mentioned this pull request Jan 22, 2025
Closed
@r100-stack r100-stack marked this pull request as ready for review January 22, 2025 15:58
@r100-stack r100-stack requested a review from a team as a code owner January 22, 2025 15:58
@r100-stack r100-stack requested review from mayank99 and smmr-dn and removed request for a team January 22, 2025 15:59
mayank99
mayank99 previously approved these changes Jan 22, 2025
View reviewed changes
packages/itwinui-react/src/styles.js/vite.config.mjs Outdated Show resolved Hide resolved
@r100-stack
Copy link
Member Author

r100-stack commented Jan 24, 2025 •
edited
Loading

Update: I recreated the lock file without deduping vite. This is because trying to dedupe caused all css-workshop tests to fail or timeout. So, I added the deduping part as an after PR TODO.

Just fyi, I tried to dedupe just vite by doing the following:

  • Add a resolution in the root package.json to have all vite@5 use [email protected]
  • Run pnpm i
  • Undo the resolution change
  • Run pnpm i

@mayank99
Copy link
Contributor

Update: I recreated the lock file without deduping vite.

Can you clarify which versions of vite we are using now? The PR title is misleading if we don't have a single 5.4.14 version.

@r100-stack
Copy link
Member Author

Update: I recreated the lock file without deduping vite.

Can you clarify which versions of vite we are using now? The PR title is misleading if we don't have a single 5.4.14 version.

Ohh, I see. The versions in the package-lock are:

I kept the title as-is for now even though 5.1.8 doesn't fall within ~5.4.14 since I guess 5.1.8 is not coming from @itwin/itwinui-react.

mayank99
mayank99 reviewed Jan 28, 2025
View reviewed changes
pnpm-lock.yaml Outdated Show resolved Hide resolved
@mayank99 mayank99 dismissed their stale review January 28, 2025 16:07

PR changed significantly since approval.

mayank99
mayank99 approved these changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@mayank99 mayank99 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just make sure to update PR description

I'm noticing CI is failing for react visual tests, but I think that's caused by #2391; maybe one of the Toast images needs to be updated to be less flaky.

@r100-stack r100-stack changed the title Bump vite to ~5.4.14 Bump vite to 5.4.14 Jan 30, 2025
@r100-stack r100-stack mentioned this pull request Jan 30, 2025
Merged
2 tasks
@r100-stack
Copy link
Member Author

r100-stack commented Jan 30, 2025 •
edited
Loading

LGTM, just make sure to update PR description

Sounds good, updated the PR description. Also updated the PR title for clarity.

I'm noticing CI is failing for react visual tests, but I think that's caused by #2391; maybe one of the Toast images needs to be updated to be less flaky.

Ohh, I see. Will take a look. Added it as an after PR TODO in #2391.

@r100-stack r100-stack merged commit e614a0e into main Jan 30, 2025
18 checks passed
@r100-stack r100-stack deleted the r/vite-5.4 branch January 30, 2025 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smmr-dn smmr-dn smmr-dn approved these changes

@mayank99 mayank99 mayank99 approved these changes

Assignees

@r100-stack r100-stack

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@r100-stack @mayank99 @smmr-dn